-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
flowinfra: disable queueing mechanism of the flow scheduler by default #85091
Conversation
This commit disables the queueing mechanism of the flow scheduler as part of the effort to remove that queueing altogether during 23.1 release cycle. To get there though we choose a conservative approach of introducing a cluster setting that determines whether the queueing is enabled or not, and if it is disabled, then we effectively a treating `sql.distsql.max_running_flows` limit as infinite. By default, the queueing is now disabled since recent experiments have shown that the admission control does a good job of protecting the nodes from the influx of remote flows. Release note: None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cdc is 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @yuzefovich)
pkg/sql/flowinfra/cluster_test.go
line 389 at r1 (raw file):
t.Fatal(err) } }
Why was this query removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @mgartner)
pkg/sql/flowinfra/cluster_test.go
line 389 at r1 (raw file):
Previously, cucaroach (Tommy Reilly) wrote…
Why was this query removed?
I don't think it's needed here. Probably it was introduced to populate the range cache, but we are manually constructing the physical plan, so the range cache doesn't matter. This test if effectively a copy of TestClusterFlow
, but in the tenant world, and in there we don't have such a query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @yuzefovich)
pkg/sql/flowinfra/cluster_test.go
line 389 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
I don't think it's needed here. Probably it was introduced to populate the range cache, but we are manually constructing the physical plan, so the range cache doesn't matter. This test if effectively a copy of
TestClusterFlow
, but in the tenant world, and in there we don't have such a query.
👍
TFTRs! bors r+ |
Build succeeded: |
This commit disables the queueing mechanism of the flow scheduler as
part of the effort to remove that queueing altogether during 23.1
release cycle. To get there though we choose a conservative approach of
introducing a cluster setting that determines whether the queueing is
enabled or not, and if it is disabled, then we effectively a treating
sql.distsql.max_running_flows
limit as infinite. By default, thequeueing is now disabled since recent experiments have shown that the
admission control does a good job of protecting the nodes from the
influx of remote flows.
Addresses: #34229.
Release note: None